[CI] Support package discovery in nested directories - WIP#18495
[CI] Support package discovery in nested directories - WIP#18495mrodm wants to merge 30 commits intoelastic:mainfrom
Conversation
- Add ListPackages function to dev/citools/packages.go that walks a directory and returns sorted paths of all valid packages found. - Extend packageManifest struct with FormatVersion, Type and Version fields, and add IsValid() method mirroring dev/packagenames logic. - Refactor dev/packagenames to delegate package discovery to citools.ListPackages, removing the now-redundant walkPackagePaths. Generated with Claude (claude-sonnet-4-6)
- Rename readPackageManifest to ReadPackageManifest so it can be used from other packages. - Remove dev/packagenames/manifest.go and its local packageManifest type, replacing all usages with citools.ReadPackageManifest. - Update all internal callers (kibana.go, logsdb.go, subscription.go, packages.go) to use the renamed function. Generated with Claude (claude-sonnet-4-6)
- Replace the inline WalkDir loop in validatePackages with a call to citools.ListPackages, removing duplicated package-discovery logic. - Update testdata manifests (devexp, package_1) with the required format_version, name, type and version fields so they are recognised as valid packages by citools.ListPackages/IsValid. Generated with Claude (claude-sonnet-4-6)
- Add nested_packages testdata with packages at two directory levels: one at the top level and two under a category/ subdirectory, one of which has data streams. - Add five new TestValidatePackages cases covering: all packages with explicit owners, a missing owner for a nested package, category-level ownership inherited by nested packages, valid per-stream ownership in a nested package, and missing stream owner in a nested package. Generated with Claude (claude-sonnet-4-6)
Add a ListPackages target that calls citools.ListPackages to print all package paths found under the packages directory, one per line. Generated with Claude (claude-sonnet-4-6)
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| list_all_directories() { | ||
| find . -maxdepth 1 -mindepth 1 -type d | xargs -I {} basename {} | sort | ||
| mage listPackages | grep -E '^packages/(elastic_package_registry|nginx)$' | ||
| } |
There was a problem hiding this comment.
🔴 Critical scripts/common.sh:831
list_all_directories() now hardcodes a grep filter that returns only two package names (elastic_package_registry and nginx). Since callers trigger_integrations_in_parallel.sh and test_integrations_with_serverless.sh iterate over this output to drive CI pipelines, all other packages are silently excluded from testing and publishing. If this restriction is intentional, consider documenting why only these two packages should run; otherwise, the filter should be removed to restore full package coverage.
list_all_directories() {
- mage listPackages | grep -E '^packages/(elastic_package_registry|nginx)$'
+ mage listPackages
}🤖 Copy this AI Prompt to have your agent fix this:
In file .buildkite/scripts/common.sh around lines 831-833:
`list_all_directories()` now hardcodes a grep filter that returns only two package names (`elastic_package_registry` and `nginx`). Since callers `trigger_integrations_in_parallel.sh` and `test_integrations_with_serverless.sh` iterate over this output to drive CI pipelines, all other packages are silently excluded from testing and publishing. If this restriction is intentional, consider documenting why only these two packages should run; otherwise, the filter should be removed to restore full package coverage.
There was a problem hiding this comment.
This will be removed before merging the PR. This grep is there to test just a subset of packages instead of the ~400 packages.
- Add -d "${WORKSPACE}" to the mage call in list_all_directories so it
always finds magefile.go regardless of the working directory when invoked.
- Remove pushd packages / popd wrappers in trigger_integrations_in_parallel.sh
and test_integrations_with_serverless.sh since list_all_directories now
returns full paths (e.g. packages/nginx) relative to the repo root.
- Fix process_package: the parameter rename from package to package_path was
incomplete — pushd, teardown calls, and log lines still referenced the
old ${package} variable (now unbound). Use ${package_path} for directory
operations and ${package_name} (from package_name_manifest) for labels.
- Update test_one_package.sh to derive parent_path and package_name from
the received package_path instead of assuming a packages/ prefix.
- Consistently rename local variables from package to package_name across
check_package, build_zip_package, teardown_*_package, run_tests_package,
and related helpers.
Generated with Claude (claude-sonnet-4-6)
- Export manifestFileName as ManifestFileName in dev/citools/packages.go so it can be reused across packages. - Remove the duplicate manifestFileName constant from dev/packagenames and replace its usage with citools.ManifestFileName. - Replace the hardcoded "manifest.yml" string in dev/codeowners with citools.ManifestFileName. Generated with Claude (claude-sonnet-4-6)
Generated with Claude (claude-sonnet-4-6)
mrodm
force-pushed
the
allow-multiple-directories
branch
from
17332f9 to
e2454f2
Compare
Extract the pushd/is_pr_affected/popd pattern with error handling into a reusable should_test_package function, to be used from test_integrations_with_serverless.sh and trigger_integrations_in_parallel.sh. Generated with the assistance of Claude Sonnet 4.6
🚀 Benchmarks reportTo see the full report comment with |
Replace find-based flat directory loop with list_all_directories so that packages organised in subdirectories are discovered correctly. Also replace `cat manifest.yml | yq` with direct `yq manifest.yml` calls. Generated with the assistance of Claude Sonnet 4.6
|
|
||
| buildkite-agent pipeline upload "${PIPELINE_FILE}" | ||
| cat "${PIPELINE_FILE}" | ||
| # buildkite-agent pipeline upload "${PIPELINE_FILE}" |
There was a problem hiding this comment.
🔴 Critical scripts/build_packages.sh:159
The buildkite-agent pipeline upload command is replaced with cat, so the pipeline that signs and publishes packages is never uploaded to Buildkite. Built packages will remain in artifacts but never reach the package registry.
cat "${PIPELINE_FILE}"
-# buildkite-agent pipeline upload "${PIPELINE_FILE}"
+buildkite-agent pipeline upload "${PIPELINE_FILE}"🤖 Copy this AI Prompt to have your agent fix this:
In file .buildkite/scripts/build_packages.sh around lines 159-161:
The `buildkite-agent pipeline upload` command is replaced with `cat`, so the pipeline that signs and publishes packages is never uploaded to Buildkite. Built packages will remain in artifacts but never reach the package registry.
There was a problem hiding this comment.
This is a change to allow debugging the script safely. This will be reverted before merging the PR.
mrodm
force-pushed
the
allow-multiple-directories
branch
from
c280270 to
71e903e
Compare
| if skipPublishing ; then | ||
| echo "packageStoragePublish: not the main branch or a backport branch, nothing will be published" | ||
| exit 0 | ||
| # exit 0 | ||
| fi |
There was a problem hiding this comment.
🟡 Medium scripts/build_packages.sh:88
When skipPublishing returns true, the script prints "nothing will be published" but continues executing instead of exiting. This causes the script to proceed through expensive build steps (tool installation, package building, artifact copying, pipeline generation) on branches that should skip publishing entirely, wasting resources and potentially causing side effects.
if skipPublishing ; then
echo "packageStoragePublish: not the main branch or a backport branch, nothing will be published"
- # exit 0
+ exit 0
fi🤖 Copy this AI Prompt to have your agent fix this:
In file .buildkite/scripts/build_packages.sh around lines 88-91:
When `skipPublishing` returns true, the script prints "nothing will be published" but continues executing instead of exiting. This causes the script to proceed through expensive build steps (tool installation, package building, artifact copying, pipeline generation) on branches that should skip publishing entirely, wasting resources and potentially causing side effects.
There was a problem hiding this comment.
Changes related to debugging. It will be reverted before merging.
| SIGNING_STEP_KEY: "sign-service" | ||
| ARTIFACTS_FOLDER: "packageArtifacts" | ||
| DRY_RUN: "${DRY_RUN}" | ||
| DRY_RUN: "true" |
There was a problem hiding this comment.
🟡 Medium scripts/build_packages.sh:150
DRY_RUN is hardcoded to "true" on line 150, so the trigger_publish_packages.sh step always runs in dry-run mode regardless of the DRY_RUN environment variable value. Packages will never actually be published when the pipeline intends to run for real. Consider reverting to "${DRY_RUN}" to respect the environment variable.
| DRY_RUN: "true" | |
| DRY_RUN: "${DRY_RUN}" |
🤖 Copy this AI Prompt to have your agent fix this:
In file .buildkite/scripts/build_packages.sh around line 150:
`DRY_RUN` is hardcoded to `"true"` on line 150, so the `trigger_publish_packages.sh` step always runs in dry-run mode regardless of the `DRY_RUN` environment variable value. Packages will never actually be published when the pipeline intends to run for real. Consider reverting to `"${DRY_RUN}"` to respect the environment variable.
There was a problem hiding this comment.
Changes related to debugging. It will be reverted before merging.
Replace with_go with with_mage so that mage is available when list_all_directories is called. with_mage already calls with_go internally so Go setup is preserved. Generated with the assistance of Claude Sonnet 4.6
|
|
||
| if skipPublishing ; then | ||
| echo "packageStoragePublish: not the main branch or a backport branch, nothing will be published" | ||
| exit 0 | ||
| # exit 0 | ||
| fi | ||
|
|
There was a problem hiding this comment.
🟡 Medium scripts/build_packages.sh:87
When skipPublishing returns true, the script prints "nothing will be published" but continues executing instead of exiting. It proceeds to build packages, copy artifacts, and generate the signing/publishing pipeline YAML, wasting CI resources on non-main/non-backport branches. If the DRY_RUN or buildkite-agent pipeline upload guards are re-enabled later, packages could be signed and published from arbitrary feature branches.
@@ -88,5 +88,5 @@
if skipPublishing ; then
echo "packageStoragePublish: not the main branch or a backport branch, nothing will be published"
- # exit 0
+ exit 0
fi
add_bin_path🤖 Copy this AI Prompt to have your agent fix this:
In file .buildkite/scripts/build_packages.sh around lines 87-92:
When `skipPublishing` returns true, the script prints "nothing will be published" but continues executing instead of exiting. It proceeds to build packages, copy artifacts, and generate the signing/publishing pipeline YAML, wasting CI resources on non-main/non-backport branches. If the `DRY_RUN` or `buildkite-agent pipeline upload` guards are re-enabled later, packages could be signed and published from arbitrary feature branches.
There was a problem hiding this comment.
Changes related to debugging. It will be reverted before merging.
Add a termination check for ownerDir == "/" to prevent an infinite loop
when filepath.Dir reaches the filesystem root on non-Windows systems,
where filepath.Dir("/") returns "/" instead of ".".
Generated with the assistance of Claude Sonnet 4.6
mrodm
force-pushed
the
allow-multiple-directories
branch
from
542d646 to
351491d
Compare
| if [ "${DRY_RUN}" == "true" ]; then | ||
| echo "DRY_RUN enabled. Publish packages steps skipped." | ||
| exit 0 | ||
| # exit 0 |
There was a problem hiding this comment.
🟡 Medium scripts/build_packages.sh:119
When DRY_RUN is true, the script prints the skip message but does not exit, so execution continues into the signing/publishing pipeline generation. If the commented buildkite-agent pipeline upload is re-enabled, a dry run would incorrectly trigger live signing/publishing. Consider uncommenting exit 0 to ensure dry runs actually skip the pipeline steps.
| # exit 0 | |
| exit 0 |
🤖 Copy this AI Prompt to have your agent fix this:
In file .buildkite/scripts/build_packages.sh around line 119:
When `DRY_RUN` is `true`, the script prints the skip message but does not exit, so execution continues into the signing/publishing pipeline generation. If the commented `buildkite-agent pipeline upload` is re-enabled, a dry run would incorrectly trigger live signing/publishing. Consider uncommenting `exit 0` to ensure dry runs actually skip the pipeline steps.
There was a problem hiding this comment.
This change will be removed before merging. It was required to be able to test this script/process
…port script Remove the PACKAGE_FOLDER_NAME input parameter from the backport pipeline. Instead, derive the package path automatically from PACKAGE_NAME by introducing a get_package_path function that iterates all packages via `mage listPackages` and matches on the `name` field in manifest.yml. Also refactor removeOtherPackages to accept the package path as an explicit parameter instead of relying on a global variable. Generated with the assistance of Claude Sonnet 4.6
💚 Build Succeeded
History
cc @mrodm |
mrodm
force-pushed
the
allow-multiple-directories
branch
from
351491d to
ac65fe1
Compare
…Packages Replace basename-based sed patterns with the full package path so that nested packages (e.g. packages/category/nginx) are correctly removed from CODEOWNERS. Use | as sed delimiter to avoid escaping slashes. Generated with the assistance of Claude Sonnet 4.6
Add || true to the get_package_path command substitution so a non-zero return does not trigger set -e before the -z check can run the friendly error annotation. Generated with the assistance of Claude Sonnet 4.6
Vale Linting ResultsSummary: 8 warnings, 5 suggestions found
|
| File | Line | Rule | Message |
|---|---|---|---|
| packages/nginx/nginx/_dev/build/docs/README.md | 65 | Elastic.DontUse | Don't use 'Please'. |
| packages/nginx/nginx/_dev/build/docs/README.md | 81 | Elastic.DontUse | Don't use 'Please'. |
| packages/nginx/nginx/_dev/build/docs/README.md | 90 | Elastic.DontUse | Don't use 'Please'. |
| packages/nginx/nginx/_dev/build/docs/README.md | 111 | Elastic.DontUse | Don't use 'Please'. |
| packages/nginx/nginx/docs/README.md | 181 | Elastic.DontUse | Don't use 'Please'. |
| packages/nginx/nginx/docs/README.md | 300 | Elastic.DontUse | Don't use 'Please'. |
| packages/nginx/nginx/docs/README.md | 326 | Elastic.DontUse | Don't use 'Please'. |
| packages/nginx/nginx/docs/README.md | 427 | Elastic.DontUse | Don't use 'Please'. |
💡 Suggestions (5)
| File | Line | Rule | Message |
|---|---|---|---|
| packages/nginx/nginx/_dev/build/docs/README.md | 32 | Elastic.WordChoice | Consider using 'refer to if it's a document, view if it's a UI element' instead of 'see', unless the term is in the UI. |
| packages/nginx/nginx/_dev/build/docs/README.md | 48 | Elastic.WordChoice | Consider using 'deactivate, deselect, hide, turn off' instead of 'disable', unless the term is in the UI. |
| packages/nginx/nginx/docs/README.md | 32 | Elastic.WordChoice | Consider using 'refer to if it's a document, view if it's a UI element' instead of 'see', unless the term is in the UI. |
| packages/nginx/nginx/docs/README.md | 38 | Elastic.WordChoice | Consider using 'refer to if it's a document, view if it's a UI element' instead of 'see', unless the term is in the UI. |
| packages/nginx/nginx/docs/README.md | 48 | Elastic.WordChoice | Consider using 'deactivate, deselect, hide, turn off' instead of 'disable', unless the term is in the UI. |
The Vale linter checks documentation changes against the Elastic Docs style guide.
To use Vale locally or report issues, refer to Elastic style guide for Vale.
| list_all_directories() { | ||
| find . -maxdepth 1 -mindepth 1 -type d | xargs -I {} basename {} | sort | ||
| mage -d "${WORKSPACE}" listPackages | grep -E '^packages/(elastic_package_registry|nginx|rapid7_insightvm|sql_input|nginx/nginx|nginx/nginx_otel)$' | ||
| } |
There was a problem hiding this comment.
🟠 High scripts/common.sh:865
list_all_directories() at line 866 hardcodes a grep -E filter that restricts output to only 6 specific packages, silently skipping all other packages from the build/publish and serverless testing pipelines. This appears to be a development filter accidentally committed in place of the previous find implementation that listed all package directories. Consider reverting to the previous implementation or using a more appropriate mechanism for temporary filtering.
+# list all directories that are packages from the root of the repository
list_all_directories() {
- mage -d "${WORKSPACE}" listPackages | grep -E '^packages/(elastic_package_registry|nginx|rapid7_insightvm|sql_input|nginx/nginx|nginx/nginx_otel)$'
+ find . -maxdepth 1 -mindepth 1 -type d | xargs -I {} basename {} | sort
}🤖 Copy this AI Prompt to have your agent fix this:
In file .buildkite/scripts/common.sh around lines 865-867:
`list_all_directories()` at line 866 hardcodes a `grep -E` filter that restricts output to only 6 specific packages, silently skipping all other packages from the build/publish and serverless testing pipelines. This appears to be a development filter accidentally committed in place of the previous `find` implementation that listed all package directories. Consider reverting to the previous implementation or using a more appropriate mechanism for temporary filtering.
There was a problem hiding this comment.
All this filtering of packages is related to run different testing in CI pipelines. It will be removed that grep and keep the mage -d ${WORKSPACE} listPackages command before merging this PR.
There was a problem hiding this comment.
Thanks for confirming. Would you like me to remove the grep filter and restore the command to just mage -d "${WORKSPACE}" listPackages, or will you handle this yourself?
💚 Build Succeeded
History
cc @mrodm |
andrewkroh
added
Integration:nginx
4 participants
Summary
This PR centralises package discovery logic into a new
citools.ListPackagesfunction and uses it acrosspackagenamesandcodeowners, replacing the previously duplicatedfilepath.WalkDirimplementations. As a side effect, both checks now support packages organised in nested directory structures (e.g.packages/category/my_package), not just flatpackages/my_packagelayouts.Proposed commit
WHAT:
Allow packages to be placed in nested subdirectories under
packages/(e.g.
packages/<category>/<name>) instead of being restricted to the flatpackages/<name>layout. Previously,codeownersdiscovery logic would silently ignore any package not sitting directly under
packages/, meaning nested packages would bypass ownership validation entirely.WHY:
As the number of integrations grows, being able to organise packages into
category subdirectories becomes necessary to keep the repository manageable.
The existing tooling hard-coded a single-level walk, blocking this. This change
makes nested directory layouts a supported and validated structure across all
CI tooling, so teams can start grouping packages without breaking checks.
Changes
dev/citoolsListPackages(dir string) ([]string, error)— walks a directory recursively, returns sorted paths of all valid packages found. A package is considered valid when itsmanifest.ymlcontains non-emptyformat_version,name,type(one ofintegration,input,content) andversionfields.packageManifeststruct withFormatVersion,TypeandVersionfields, and add anIsValid()method, mirroring the logic previously only present indev/packagenames.readPackageManifestasReadPackageManifestso it can be reused from other packages.dev/packagenamesmanifest.goand its localpackageManifesttype; replace all usages withcitools.ReadPackageManifest.walkPackagePathsfunction with a call tocitools.ListPackages.dev/codeownersfilepath.WalkDirloop invalidatePackageswith a call tocitools.ListPackages, removing the duplicate discovery logic.format_version,name,typeandversionfields.nested_packagestestdata fixture with packages at two directory levels (top-level and under acategory/subdirectory, one with data streams).TestValidatePackagescases covering: explicit per-package owners, missing owner for a nested package, category-level inherited ownership, valid per-stream ownership in a nested package, and missing stream owner in a nested package.magefile.goListPackagesmage target that prints all package paths under thepackages/directory, one per line..buildkite/scripts/common.shshould_test_packagehelper function that encapsulates the pushd/is_pr_affected/popdpattern along with fatal-error handling, replacing near-identical blocks that were duplicated intest_integrations_with_serverless.shandtrigger_integrations_in_parallel.sh`..buildkite/scripts/build_packages.shfind-based flat directory loop withlist_all_directories(backed bymage listPackages) so packages in nested subdirectories are discovered correctly.with_gowithwith_mage(which callswith_gointernally) somageis available whenlist_all_directoriesruns.Backport pipeline
PACKAGE_FOLDER_NAMEas a required input parameter frompipeline.backport.ymlandbackport_branch.sh. The package path is now resolved automatically by a newget_package_pathfunction that looks up the package whosemanifest.ymlnamefield matches the givenPACKAGE_NAME, usingmage listPackagesto iterate all discovered packages.docs/extend/developer-workflow-support-old-package.mdto reflect the removed parameter.Checklist
changelog.ymlfile.Author's Checklist
mage checkpasses locally with no errorsmage ListPackagescorrectly lists all packages,including any nested under a subdirectory
in nested directories correctly
list_all_directories..buildkite/scripts/build_packages.shHow to test this PR locally
Related issues